-
Notifications
You must be signed in to change notification settings - Fork 59
feat: Add <FocusTrap> docs
#258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
|
|
||
| :::note | ||
| All the examples have controls to enable / disable the focus trap so that page keyboard navigation isn't blocked. | ||
| ::: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit: this note seems a little overkill with the accessibility warning shown above. But if you are worried about people removing the logic to exit the focus trap functionality I think it is fine to keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep it since it's important for customers to implement exit logic. And <FocusTrap> can't determine if exit logic has been added. If not implemented, keyboard nav users can be completely blocked resulting in critical bugs.
hcopp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good! Just have the one doc site comment, happy to approve once you bump versions.
680b8c5 to
c862078
Compare
What changed? Why?
This PR adds docs pages for the
<FocusTrap>component. This component is used more as a util component for internal CDS component composition. However, the component is fully functional and can be used by CDS customers. Therefore, a proper documentation page should be added for it.Also, a bug in
<FocusTrap>is fixed where when theincludeTriggerInFocusTrapprop was set totrue, the focus would still shift to inside the<FocusTrap>.Root cause (required for bugfixes)
The root cause of the bug is in the last
useEffectin the component where the initial auto focus logic is. There was no check for theincludeTriggerInFocusTrapprop and the query for focusable elements doesn't include the trigger. This PR's fix updates the query so that the trigger is included in the focusable elements array when the prop istrue.UI changesTesting
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false